-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collection expressions: type inference from spread elements #69856
Conversation
@@ -2675,6 +2675,135 @@ static void Main(string[] args) | |||
Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "F").WithArguments("Program.F<T>(System.Func<T[]>)").WithLocation(9, 17)); | |||
} | |||
|
|||
[Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workitem.
@@ -615,6 +615,10 @@ private void MakeExplicitParameterTypeInferences(Binder binder, BoundExpression | |||
{ | |||
MakeCollectionExpressionTypeInferences(binder, (BoundUnconvertedCollectionExpression)argument, target, kind, ref useSiteInfo); | |||
} | |||
else if (argument.Kind == BoundKind.CollectionExpressionSpreadElement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it super confusing that this parameter's name is argument
when it seems like it might be any subexpression of an argument. Not for this PR to fix, though.
} | ||
|
||
// https://github.com/dotnet/roslyn/issues/68786: Ignoring top-level nullability of enumeratorInfo.ElementType. | ||
LowerBoundInference(TypeWithAnnotations.Create(enumeratorInfo.ElementType), target, ref useSiteInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use ElementTypeWithAnnotations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this could break something and cause us to not infer a type as nullable where we should. Please make sure to test something like:
M([..[default(string?)]]);
void M<T>(T[] arr);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I hadn't noticed ElementTypeWithAnnotations
previously.
I've added a couple of tests for inferring nullability but we are still not handling collection expressions in NullableWalker. If possible, I'd prefer to handle this when fixing #68786.
dynamic x = new[] { 2, 3 }; | ||
var y = F([..x]); | ||
y.Report(includeType: true); | ||
var z = F([1, ..x]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be nice to show a case with multiple spread arguments. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to TypeInference_Spread_01
.
CompileAndVerify(new[] { source, s_collectionExtensions }, expectedOutput: "(System.Object[]) [1, 2, 3], "); | ||
} | ||
|
||
// If we allow inference from a spread element _expression_ rather than simply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would def want to do this eventually. I feel like if we can make return [null]
"just work" then return [..[null]]
should also just work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass
See Type inference:
Fixes #69706
Relates to test plan #66418